Adds separate configs for attributes and parameters by d-Pixie · Pull Request #1 · accodeing/rest-easy

d-Pixie · 2026-03-25T14:15:02Z

Both the checkbiz and tradera gems needed to implement a workaround for attributes in the JSON being of a different form than the query parameters. To facilitate this in the base gem a new config option conversions with two sub keys: query_parameters and json_attributes that work the same as the old attribute_conversion setting, but for the two different cases.

Copilot

Pull request overview

Introduces a new conversions configuration (with json_attributes and query_parameters) to support APIs that use different naming conventions for JSON bodies vs query parameters, while deprecating the old attribute_convention setting.

Changes:

Add nested conversions config to module settings and per-Resource config, plus new json_attribute_converter / query_parameter_converter resolution.
Automatically transform Resource.get(params: ...) query parameter keys using the configured query_parameters convention.
Update specs/docs, add a conversions spec suite, add changelog entry, and bump gem version to 1.1.0.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
spec/rest_easy/resource_spec.rb	Updates many specs to explicitly set `conversions.json_attributes` so existing PascalCase expectations still pass.
spec/rest_easy/resource/inheritance_spec.rb	Updates inheritance test to use `config.conversions.json_attributes`.
spec/rest_easy/resource/http_spec.rb	Sets module config for `conversions.json_attributes` in HTTP integration specs.
spec/rest_easy/resource/crud_spec.rb	Updates CRUD spec module config to use `conversions.json_attributes`.
spec/rest_easy/resource/conversions_spec.rb	Adds new coverage for module-level/resource-level overrides, independence of the two conversions, and deprecation behavior.
lib/rest_easy/version.rb	Bumps gem version to `1.1.0`.
lib/rest_easy/settings.rb	Adds `conversions` settings and deprecates `attribute_convention` at the module level.
lib/rest_easy/resource.rb	Implements converter resolution and applies query parameter conversion in `get`.
lib/rest_easy.rb	Adds BC propagation from deprecated `attribute_convention` to `conversions.json_attributes`.
README.md	Updates documentation to the new `conversions.*` configuration and adds query-params behavior notes.
Gemfile.lock	Updates path gem version to `1.1.0`.
CHANGELOG.md	Adds `1.1.0` entry documenting conversions and deprecation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-26T05:49:17Z

lib/rest_easy/resource.rb

+        params.transform_keys! { |k| query_parameter_converter.serialise(k) }
        parent.get(path:, params:, headers:)


params.transform_keys! mutates the caller-provided params hash and will raise if the hash is frozen. This can cause surprising side effects for callers reusing a params hash across requests. Prefer a non-mutating transform (e.g., build a new hash and pass that to parent.get) and leave the input untouched.

Suggested change

params.transform_keys! { |k| query_parameter_converter.serialise(k) }

parent.get(path:, params:, headers:)

converted_params = params.transform_keys { |k| query_parameter_converter.serialise(k) }

parent.get(path:, params: converted_params, headers:)

Copilot · 2026-03-26T05:49:17Z

lib/rest_easy.rb

+        if ac
+          warn "RestEasy: attribute_convention is deprecated, use `conversions.json_attributes = #{ac.inspect}` instead"
+          self::Settings.config.conversions.json_attributes = ac


The BC propagation always warns and unconditionally overwrites conversions.json_attributes whenever attribute_convention is present. This makes it hard to migrate gradually (a user could set conversions.json_attributes and still get it overridden) and can emit repeated warnings across multiple configure calls. Consider only propagating when conversions.json_attributes is unset, and/or clearing attribute_convention after propagation, and align warning behavior with the intended compatibility policy.

Suggested change

if ac

warn "RestEasy: attribute_convention is deprecated, use `conversions.json_attributes = #{ac.inspect}` instead"

self::Settings.config.conversions.json_attributes = ac

conv = self::Settings.config.conversions

if ac && conv.json_attributes.nil?

warn "RestEasy: attribute_convention is deprecated, use `conversions.json_attributes = #{ac.inspect}` instead"

conv.json_attributes = ac

Copilot · 2026-03-26T05:49:17Z

lib/rest_easy/settings.rb

+    setting :attribute_convention # deprecated — propagated to conversions.json_attributes in configure
+
+    setting :conversions do
+      setting :query_parameters, default: :snake_case, reader: true
+      setting :json_attributes, default: :snake_case, reader: true


This change removes the previous default attribute_convention (was :PascalCase) and sets both conversions defaults to :snake_case. That’s a breaking behavioral change for consumers who relied on the old default mapping without explicitly configuring it (and it also interacts with Resource classes that have no parent module). If backwards compatibility is a goal for a 1.x minor bump, consider keeping the old default for conversions.json_attributes (and possibly query_parameters), or bumping the major version and calling out the default change prominently in docs/changelog.

Suggested change

setting :attribute_convention # deprecated — propagated to conversions.json_attributes in configure

setting :conversions do

setting :query_parameters, default: :snake_case, reader: true

setting :json_attributes, default: :snake_case, reader: true

setting :attribute_convention, default: :PascalCase # deprecated — propagated to conversions.json_attributes in configure

setting :conversions do

setting :query_parameters, default: :snake_case, reader: true

setting :json_attributes, default: :PascalCase, reader: true

Copilot · 2026-03-26T05:49:18Z

CHANGELOG.md

+### Deprecated
+
+- **`attribute_convention`** is deprecated in favour of `conversions.json_attributes`. The old setting continues to work and is respected as a fallback, but emits a deprecation warning when used at the Resource level. Module-level `attribute_convention` is silently supported for backwards compatibility.
+


The changelog says module-level attribute_convention is "silently supported" for backwards compatibility, but RestEasy.configure currently emits a deprecation warning when it sees attribute_convention set. Either update the changelog wording or adjust the implementation to avoid warning at the module level (and only warn for resource-level usage).

Copilot · 2026-03-26T05:49:18Z

lib/rest_easy/resource.rb

+        @json_attribute_converter ||= Conventions.resolve(
+          config.conversions.json_attributes ||
+          parent&.config&.conversions&.json_attributes
+        )
+      end
+
+      def query_parameter_converter
+        @query_parameter_converter ||= Conventions.resolve(
+          config.conversions.query_parameters ||
+          parent&.config&.conversions&.query_parameters
+        )


json_attribute_converter/query_parameter_converter are memoized, but configure can be called multiple times (including after class definition) and parent module config can also change. Once memoized, changes to config.conversions.* (or inherited values) won’t be picked up, leading to stale conversion behavior. Consider either removing memoization or memoizing based on the effective convention value and resetting when configure updates conversions.

Also, if this Resource has no RestEasy parent module and no per-resource conversions set, both methods call Conventions.resolve(nil) (which returns nil), and later callers like attr will crash on nil.serialise. Add an explicit fallback (e.g. to :snake_case/previous default) when neither local nor parent config provides a convention.

Suggested change

@json_attribute_converter ||= Conventions.resolve(

config.conversions.json_attributes ||

parent&.config&.conversions&.json_attributes

)

end

def query_parameter_converter

@query_parameter_converter ||= Conventions.resolve(

config.conversions.query_parameters ||

parent&.config&.conversions&.query_parameters

)

convention =

config.conversions.json_attributes ||

parent&.config&.conversions&.json_attributes ||

:snake_case

Conventions.resolve(convention)

end

def query_parameter_converter

convention =

config.conversions.query_parameters ||

parent&.config&.conversions&.query_parameters ||

:snake_case

Conventions.resolve(convention)

ehannes

Looks solid. My concern is more about code quality and structure, which mostly is out of scope for this PR. But the Resource class is really large and does a lot. Why not split that into several classes?

I would also like to change things regarding the test setup to make it more robust. I think we are leaking setup code between test cases. We should start using Rubocop, I'm pretty sure we are violating a lot of rules :)

ehannes · 2026-03-26T05:51:41Z

spec/rest_easy/resource/conversions_spec.rb

+  def setup_test_connection(api_module, &block)
+    stubs = Faraday::Adapter::Test::Stubs.new(&block)
+    api_module.instance_variable_set(:@faraday_connection, nil)
+    api_module.connection do |f|
+      f.request :json
+      f.response :json, content_type: /\bjson$/
+      f.adapter :test, stubs
+    end
+    stubs
+  end


This looks like something that should be extracted to a spec helper. The spec is already really long.

ehannes · 2026-03-26T05:52:46Z

spec/rest_easy/resource/conversions_spec.rb

+    before(:all) do
+      module ConvTestApi
+        extend RestEasy
+
+        configure do
+          conversions.json_attributes = :PascalCase
+          conversions.query_parameters = :camelCase
+        end
+      end
+
+      class ConvTestApi::Invoice < RestEasy::Resource
+        configure { path "invoices" }
+
+        key :document_number, Integer, :read_only
+        attr :customer_name, String
+      end
+    end


Are we using Rubocop in this project? I'm pretty sure this will be flagged as leaking test setup. We should stub the class in stead of creating a mock module. We have established patterns for this in other projects.

ehannes · 2026-03-26T05:53:08Z

spec/rest_easy/resource/conversions_spec.rb

+    after(:all) do
+      Object.send(:remove_const, :ConvTestApi)
+    end


And you will get rid of this if you use stubbing instead.

ehannes · 2026-03-26T05:55:43Z

spec/rest_easy/resource/conversions_spec.rb

+    stubs
+  end
+
+  # ── Module-level configuration ──────────────────────────────────────


These comments, I'm not a fan of comments in code and especially not block level comments like these. Most often they will be outdated. Also - we already have the describe blocks.

ehannes · 2026-03-26T05:59:03Z

lib/rest_easy/resource.rb

      end

-      # -- attribute_convention ------------------------------------------
+      # -- conversions ---------------------------------------------------


What happened with small single responsibility classes? This class is huge. Also I'm not a fan of these comments. If this is a separate concern in this class then it might be a sign that we can extract it to a separate class that we can test in isolation 😄

ehannes · 2026-03-26T06:01:40Z

spec/rest_easy/resource_spec.rb

    describe "path" do
      it "sets the endpoint path via configure" do
        resource = Class.new(described_class) do
+          configure { conversions.json_attributes = :PascalCase }


This is why I dislike the test strategy where you inline everything in one it statement. As soon as you change something, like this configuration, you need to update every test for that class (which in this case is very many!). I suggest using a shared setup instead.

Adds separate configs for attributes and parameters

bbb73bc

d-Pixie requested a review from ehannes March 25, 2026 14:15

d-Pixie self-assigned this Mar 25, 2026

d-Pixie added the enhancement New feature or request label Mar 25, 2026

d-Pixie added 5 commits March 25, 2026 16:16

Updates README.md with new config structure

d21f455

Moves resolver defaults to parent module settings

4b2f10b

Proxies old module setting to new config

82d9f09

Simplifies the memoized converter functions

f540047

Simplifies query param conversion logic

8991cd7

ehannes requested a review from Copilot March 26, 2026 05:44

Copilot started reviewing on behalf of ehannes March 26, 2026 05:44 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

ehannes requested changes Mar 26, 2026

View reviewed changes

		params.transform_keys! { \|k\| query_parameter_converter.serialise(k) }
		parent.get(path:, params:, headers:)

		### Deprecated

		- `attribute_convention` is deprecated in favour of `conversions.json_attributes`. The old setting continues to work and is respected as a fallback, but emits a deprecation warning when used at the Resource level. Module-level `attribute_convention` is silently supported for backwards compatibility.

Conversation

d-Pixie commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes left a comment

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ehannes Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants